-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8367341: C2: apply KnownBits and unsigned bounds to And / Or operations #27618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…ral Value inferences
|
👋 Welcome back qamai! A progress list of the required criteria for merging this PR into |
|
@merykitty This change is no longer ready for integration - check the PR body for details. |
|
@merykitty The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
SirYwell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice change overall.
I'm not sure how "easily" we can really see the benefit in the example of the interval splitting, but I leave that to others to judge.
I was just wondering, do you think it makes sense to move more such code into the RangeInference classes in future (e.g., for shift ops) or how we'll tell what to place where. From what it looks like the main reason currently is to use the TypeIntMirror classes for testability, which other node types definitely could benefit from as well.
Without it, the simple inference function fails
Yes that is entirely my intention, that for example, we only need to implement |
|
@eme64 I think it would be great if you take a look at this PR. |
eme64
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@merykitty Thank you very much for working on this, very exciting. And it seems that the actual logic is now simpler than all the custom logic before!
However, we need to make sure that all cases that you are not deleting are indeed covered.
OrINode::add_ring
if ( r0 == TypeInt::BOOL ) {
if ( r1 == TypeInt::ONE) {
return TypeInt::ONE;
} else if ( r1 == TypeInt::BOOL ) {
return TypeInt::BOOL;
}
} else if ( r0 == TypeInt::ONE ) {
if ( r1 == TypeInt::BOOL ) {
return TypeInt::ONE;
}
}
That seems to be covered by KnownBits.
OrINode::add_ring
if (r0 == TypeInt::MINUS_1 || r1 == TypeInt::MINUS_1) {
return TypeInt::MINUS_1;
}
Seems also ok, handled by the KnownBits.
OrINode::add_ring
// If either input is not a constant, just return all integers.
if( !r0->is_con() || !r1->is_con() )
return TypeInt::INT; // Any integer, but still no symbols.
// Otherwise just OR them bits.
return TypeInt::make( r0->get_con() | r1->get_con() );
Constants would also be handeld by KnownBits.
-
xor_upper_bound_for_ranges
I think also this should be handled by doing KnownBits first, and then inferring the signed/unsigned bounds, right? -
and_value
Does not look so trivial. Maybe you can go over it step by step, and leave some GitHub code comments?
| // These allow TypeIntMirror to mimick the behaviors of TypeInt* and TypeLong*, so they can be | ||
| // passed into RangeInference methods. These are only used in testing, so they are implemented in | ||
| // the test file. | ||
| const TypeIntMirror* operator->() const; | ||
| TypeIntMirror meet(const TypeIntMirror& o) const; | ||
| bool contains(U u) const; | ||
| bool contains(const TypeIntMirror& o) const; | ||
| bool operator==(const TypeIntMirror& o) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we limit this to DEBUG_ONLY?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, it disables these gtest in product builds, however. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I see. I suppose we can keep it. Can you somehow make it clear which block it is, maybe with some start/end markers?
I was wondering if the method below is still part of it, but I don't think so. But it was unclear at first.
| #include <array> | ||
| #include <limits> | ||
| #include <type_traits> | ||
| #include <unordered_set> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know the current state of code style guide: but are we allowed to use std::unordered_set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of a better way, we have HashTable but it is terrible since the table size is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. I'll ask some folks who might know / have an anser / opinion ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It surely would be very easy, and not affect the product. But let's see what they say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They tell me it is fine, and we are already doing similar things here:
test/hotspot/gtest/jfr/test_networkUtilization.cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a review, just a drive-by comment, following up on @eme64 "They tell me its fine".
I do not think it's okay to use most standard library headers. Doing so can run into issues with things
like our forbidden function mechanism, assert macro collision, and others. My opinion is the uses in
jfr/test_networkUtilization.cpp shouldn't be there, and aren't actually necessary. I just did a spot check,
and the only "good" case I found is test_codestrings.cpp using <regex>, where there isn't any similar
functionality available in hotspot. The suggestion in the discussion @eme64 for a set is RBTree. The O(1)
lookup by a hashtable is unlikely to matter to a gtest.
There is ongoing work updating our usage (see, for example, https://bugs.openjdk.org/browse/JDK-8369186)
and how to do that in a safe and consistent manner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use RBTreeCHeap, if going the RBTree route. It's just the easiest way of using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your inputs, I have removed the usage of std::unordered_map and replaced it with RBTreeCHeap. Is using std::array here fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @merykitty,
I think that we don't use the STL because we run without exceptions and because we want our production data structures to have custom allocators, and history :-). As std::array (AFAIU) is 'just' a typed and sized T*, I think it should be fine, as long as you avoid things that might throw!
|
@eme64 Thanks for your review, I believe I have addressed all of your suggestions.
For this, from the testing POV, all the current idealization tests pass. From the theoretical POV, let me present it below: For For For |
| // The unsigned value of the result of 'and' is always not greater than both of its inputs | ||
| // since there is no position at which the bit is 1 in the result and 0 in either input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does not sound correct.
We could have ranges 0..0b1000 for both. But then both values are 0b0010, and so the result is 0b0010, which is a 1 at a position where both uhi values had zeros.
I think you need to talk about leading zeros somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this is not about the range, but about the value in an operation. I.e. If z = x & y then z u<= x && z u<= y. This leads to the fact that the upper bound of z is not larger than the upper bounds of x and y.
| // The unsigned value of the result of 'or' is always not less than both of its inputs since | ||
| // there is no position at which the bit is 0 in the result and 1 in either input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue here as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, if z = x | y then z u>= x && z u>= y. This means that the lower bound of z is not smaller than the lower bounds of x and y.
|
Nice, thanks for all the updates. I responded to some of the points above. |
|
@merykitty |
|
@eme64 I have removed the usage of |
eme64
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, that looks better already. Just looked over the diff, now going to look at the whole patch again.
eme64
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good. I'll run some internal testing now...
eme64
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@merykitty Thanks for working on this! Especially I'm happy with the extra gtest-ing that we are now able to do on the types. This optimization will be the entry point for many KnownBits optimizations, that is exciting!
This still needs a second thorough review though, since it is not trivial ;)
Co-authored-by: Emanuel Peter <[email protected]>
eme64
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still good :)
|
@merykitty this pull request can not be integrated into git checkout andorxor
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
|
May I have a second review, please? |
Hi,
This PR improves the implementation of
AndNode/OrNode/XorNode::Valueby taking advantages of the additional information inTypeInt. The implementation is pretty straightforward. A clever trick is that by analyzing the negative and positive ranges of aTypeIntseparately, we have better info for the leading bits. I also implement gtest unit tests to verify the correctness and monotonicity of the inference functions.Please take a look and leave your reviews, thanks a lot.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27618/head:pull/27618$ git checkout pull/27618Update a local copy of the PR:
$ git checkout pull/27618$ git pull https://git.openjdk.org/jdk.git pull/27618/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27618View PR using the GUI difftool:
$ git pr show -t 27618Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27618.diff
Using Webrev
Link to Webrev Comment